[Backport] Fix condition Safe-NULL condition on Update Trigger #23079

sebastien-kathmandu · 2019-05-31T10:35:16Z

Description (*)

Safe-NULL in the Update Trigger created by View is used in the wrong way. As we are using an external integration system using INSERT INTO ... ON DUPLICATE KEY UPDATE value= ... on most on our product ( for example stock, prices, varchar attribute) each integration is followed by a reindex of most of our products that are not really updated. And impact a lot the performances of our database.

Fixed Issues (if relevant)

Duplicated product is out of stock when index set to UPDATE BY SCHEDULE #15939: Duplicated product is out of stock when index set to UPDATE BY SCHEDULE
Triggers created by MView are triggered all the time #23077: Triggers created by MView are triggered all the time

Manual testing scenarios (*)

To understand the condition used before and after the commit df5597a2e and the solution proposed in this Pull Request.

I'm doing this benchmark playground :

CREATE TABLE `test_trigger_cond1` (
  `value` VARCHAR(255) DEFAULT NULL
) ENGINE=INNODB DEFAULT CHARSET=utf8;

CREATE TABLE `test_trigger_cond2` (
  `value` VARCHAR(255) DEFAULT NULL
) ENGINE=INNODB DEFAULT CHARSET=utf8;

INSERT INTO `test_trigger_cond1` (`value`)
VALUES
    ('Magento'),
    (NULL),
    ('1');

INSERT INTO `test_trigger_cond2` (`value`)
VALUES
    ('Magento'),
    (NULL),
    ('1');


SELECT
a.value "Value A",
b.value "Value B",
a.value != b.value "before commit df5597a2e4",
a.value <=> b.value "after commit df5597a2e4",
NOT a.value <=> b.value "Suggested Trigger Comparison"
FROM test_trigger_cond1 a, test_trigger_cond2 b;

here the result of the last query:

Value A	Value B	before commit `df5597a2e4`	after commit `df5597a2e4`	Suggested Trigger Comparison
Magento	Magento	0	1	0
NULL	Magento	NULL	0	1
1	Magento	1	0	1
Magento	NULL	NULL	0	1
NULL	NULL	NULL	1	0
1	NULL	NULL	0	1
Magento	1	1	0	1
NULL	1	NULL	0	1
1	1	0	1	0

Before

as we can see before the commit the NULL condition could cause a lot of trouble if for example a previous attribute were NULL or become NULL as change the

After

Only if the both value are the same we got true.

if we look a classic line in a trigger for example : trg_catalog_product_entity_int_after_update

BEGIN
SET @entity_id = (SELECT `entity_id` FROM `catalog_product_entity` WHERE `row_id` = NEW.`row_id`);
...
IF (
NEW.`value_id` <=> OLD.`value_id` OR 
NEW.`attribute_id` <=> OLD.`attribute_id` OR 
NEW.`store_id` <=> OLD.`store_id` OR 
NEW.`row_id` <=> OLD.`row_id` OR 
NEW.`value` <=> OLD.`value`
) THEN 
INSERT IGNORE INTO `catalog_product_flat_cl` (`entity_id`) values(@entity_id); END IF;
...
END

if at least one of these fields (value_id, attribute_id, store_id, row_id, value) didn't change we will always insert a new row inside catalog_product_flat_cl. And this is always the case.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

m2-assistant · 2019-05-31T10:35:19Z

Hi @sebastien-kathmandu. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

magento-cicd2 · 2019-05-31T10:35:20Z

All committers have signed the CLA.

sebastien-kathmandu · 2019-05-31T10:51:38Z

I forgot to specify that it impacts also the branch 2-3-develop

sebastien-kathmandu · 2019-05-31T11:34:02Z

@magento give me 2.2-develop instance
test issue #15939

magento-engcom-team · 2019-05-31T11:34:03Z

Hi @sebastien-kathmandu. Thank you for your request. I'm working on Magento 2.2-develop instance for you

magento-engcom-team · 2019-05-31T11:34:51Z

Hi @sebastien-kathmandu, here is your Magento instance.
Admin access: https://i-23079-2-2-develop.instances.magento-community.engineering/admin
Login: admin Password: 123123q
Instance will be terminated in up to 3 hours.

orlangur · 2019-06-01T01:27:01Z

@akaplya would be really nice if you can review this MView/Staging logic change.

rodrigowebjump · 2019-06-04T13:54:12Z

Hi @sebastien-kathmandu Did you open a PR for 2.3 branch?

sebastien-kathmandu · 2019-06-04T21:26:06Z

Hi @rodrigowebjump ,

We are still using 2.2-develop at work so I did it from 2.3-develop. Do you need a rebase or can you work with it?

Cheers,

QuentinFarizon · 2019-06-04T22:15:06Z

Thanks @sebastien-kathmandu for this fix !

I think that triggers should be reinstalled after applying this as a patch, or upgrading to a version that contains it, no ?

In the absence of a command to do that, or even proper documentation (see #23023 and magento/devdocs#4647 ), should this PR contain a setupUpgrade script that reinstalls all triggers ? Is that even doable ?

Anyway, a bin/magento indexer:triggers:reinstall is badly needed I think

sebastien-kathmandu · 2019-06-04T22:40:17Z

Hi @QuentinFarizonAfrimarket ,

It's a really good point. We did in our environment by switching the indexes to On save and back to On schedule and so the triggers are regenerated. I should do a SetupUgrade indeed running the command that regenerate them. I'll look at it ASAP. in the same time the command line can be a good idea.

Cheers,

paales · 2019-06-07T07:54:42Z

There is Magento\Indexer\Setup\RecurringData, doesn't that already remove all and add all triggers?

QuentinFarizon · 2019-06-07T15:41:08Z

Yes, Magento\Indexer\Setup\RecurringData seems to do what is needed here.
Not sure if we can call it from a schema upgrade and/or a cli command.

paales · 2019-06-07T20:04:56Z

Its called every upgrade already :)

sebastien-kathmandu · 2019-06-08T02:34:56Z

Hi @paales and @QuentinFarizonAfrimarket ,

I looked and indeed it should do it. But when we deployed the fix into our staging and production environment, the cli setup:upgrade didn't update the triggers, I had to switch to on Save and back to Schedule my indexers. I'll try it on my local to see why. but hopefully you're right guys ;-).

Cheers,

hostep · 2019-06-17T17:32:13Z

@sebastien-kathmandu: it might be better to first submit a PR which targets the 2.3-develop branch, otherwise this PR is not going to be processed very fast I'm afraid. Thanks! 🙂

(for the people interested: there is also some discussion going on around this PR and the history of this bug in https://magentocommeng.slack.com/archives/C4YS78WE6/p1560763042380000)

sebastien-kathmandu · 2019-06-17T20:35:54Z

Hi @hostep,

I created this PR : #23294 which target 2.3-develop branch. if it could be reviewed.

Next time I'll do it from 2.3 straight that could win time in the process.

Cheers,

Sebastien

mattheo-geoffray · 2019-06-18T12:32:25Z

For people looking at this PR there is also this logic in a Catalog Staging class in the Commerce version

sebastien-kathmandu · 2019-06-18T21:13:34Z

Hi @mattheo-geoffray ,

Indeed in the file src/vendor/magento/module-catalog-staging/Model/Mview/View/Attribute/Subscription.php when using composer Line 134 in my case.

Thanks @mattheo-geoffray .

Cheers,

VladimirZaets · 2019-07-10T17:21:18Z

Hi @sebastian-virtua. The branch 2.2-develop will be closed for contribution from July 15, 2019. Please finish all your Pull Requests till this date, otherwise, they will be closed.
Thanks for collaboration.

VladimirZaets · 2019-07-15T22:27:20Z

Hi @sebastien-kathmandu
The next 2.2 release will be the last of functional releases.
Due to it, we are closing 2.2-develop for contribution and all not ready for delivery Pull Requests.
Thanks for collaboration.

m2-assistant · 2019-07-15T22:27:22Z

Hi @sebastien-kathmandu, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Fix condition Safe-NULL condition for Trigger

444bc8d

magento-engcom-team added Component: Mview Release Line: 2.2 labels May 31, 2019

rodrigowebjump self-requested a review June 4, 2019 13:50

QuentinFarizon mentioned this pull request Jun 6, 2019

Cron job not running after crashed once #23054

Closed

VladimirZaets self-assigned this Jul 10, 2019

VladimirZaets added the Progress: on hold label Jul 10, 2019

VladimirZaets changed the title ~~Fix condition Safe-NULL condition on Update Trigger~~ [Backport] Fix condition Safe-NULL condition on Update Trigger Jul 10, 2019

VladimirZaets closed this Jul 15, 2019

[Backport] Fix condition Safe-NULL condition on Update Trigger #23079

[Backport] Fix condition Safe-NULL condition on Update Trigger #23079

Uh oh!

Conversation

sebastien-kathmandu commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Before

After

Contribution checklist (*)

Uh oh!

m2-assistant bot commented May 31, 2019

Uh oh!

magento-cicd2 commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastien-kathmandu commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastien-kathmandu commented May 31, 2019

Uh oh!

magento-engcom-team commented May 31, 2019

Uh oh!

magento-engcom-team commented May 31, 2019

Uh oh!

orlangur commented Jun 1, 2019

Uh oh!

rodrigowebjump commented Jun 4, 2019

Uh oh!

sebastien-kathmandu commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuentinFarizon commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastien-kathmandu commented Jun 4, 2019

Uh oh!

paales commented Jun 7, 2019

Uh oh!

QuentinFarizon commented Jun 7, 2019

Uh oh!

paales commented Jun 7, 2019

Uh oh!

sebastien-kathmandu commented Jun 8, 2019

Uh oh!

hostep commented Jun 17, 2019

Uh oh!

sebastien-kathmandu commented Jun 17, 2019

Uh oh!

mattheo-geoffray commented Jun 18, 2019

Uh oh!

sebastien-kathmandu commented Jun 18, 2019

Uh oh!

VladimirZaets commented Jul 10, 2019

Uh oh!

VladimirZaets commented Jul 15, 2019

Uh oh!

m2-assistant bot commented Jul 15, 2019

Uh oh!

Uh oh!

sebastien-kathmandu commented May 31, 2019 •

edited

Loading

magento-cicd2 commented May 31, 2019 •

edited

Loading

sebastien-kathmandu commented May 31, 2019 •

edited

Loading

sebastien-kathmandu commented Jun 4, 2019 •

edited

Loading

QuentinFarizon commented Jun 4, 2019 •

edited

Loading